Keep SMB rename verification off the UI thread#4611
Open
venkyqz wants to merge 1 commit intoTeamAmaze:release/4.0from
Open
Keep SMB rename verification off the UI thread#4611venkyqz wants to merge 1 commit intoTeamAmaze:release/4.0from
venkyqz wants to merge 1 commit intoTeamAmaze:release/4.0from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prevents a potential UI-thread network call during rename completion by limiting the post-rename exists(context) verification to SAF-backed targets only (DocumentFile/OTG), avoiding SMB (and other non-SAF) synchronous checks on the main thread.
Changes:
- Gate the post-rename
newFile.exists(context)probe behind a SAF/OTG-only predicate inMainActivityHelper. - Add a small helper (
shouldVerifyRenameWithExists) encapsulating the gating decision. - Add a focused unit test covering the SAF vs non-SAF decision behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/src/main/java/com/amaze/filemanager/utils/MainActivityHelper.java | Skips UI-thread exists(context) verification for SMB/non-SAF rename completion; keeps it for SAF/OTG where the workaround is needed. |
| app/src/test/java/com/amaze/filemanager/utils/MainActivityHelperTest.java | Adds regression coverage for the SAF/OTG-only verification decision. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
venkyqz
force-pushed
the
fix-smb-rename-main-thread
branch
from
1bbd4da to
a036ec2
Compare
The rename completion path in MainActivityHelper re-checked success by calling newFile.exists(context) on the UI thread because SAF-backed renames can report false negatives. For SMB targets that fallback routes into HybridFile.exists() and performs synchronous network I/O on the main thread. Limit the fallback probe to SAF-backed targets and cover it with a focused regression test. Constraint: Preserve the post-rename verification workaround for SAF-backed rename targets Rejected: Change HybridFile.exists() semantics for all SMB callers | too broad and would affect background operations that legitimately rely on it Confidence: high Scope-risk: narrow Directive: Keep rename completion fallbacks off the UI thread for remote backends; only use exists(context) as a SAF-specific verification path Tested: ./gradlew :app:testFdroidDebugUnitTest --tests com.amaze.filemanager.utils.MainActivityHelperTest -x kaptFdroidDebugUnitTestKotlin Tested: ./gradlew :app:compileFdroidDebugJavaWithJavac Tested: ./gradlew :app:compileFdroidDebugUnitTestJavaWithJavac -x kaptFdroidDebugUnitTestKotlin Not-tested: Full :app:testFdroidDebugUnitTest (blocked by existing kaptFdroidDebugUnitTestKotlin failure in unrelated tests referencing missing ShadowMultiDex)
venkyqz
force-pushed
the
fix-smb-rename-main-thread
branch
from
a036ec2 to
015f7cf
Compare
EmmanuelMess
requested changes
Apr 25, 2026
| * instead of merely looking at the return value | ||
| */ | ||
| if (b || newFile.exists(context)) { | ||
| if (b |
Member
There was a problem hiding this comment.
This doesn't fix the issue (exists running on main thread) and seems incredibly easy to break if the implementation of Operations.rename changes (by changing how otg is handled or using DocumentFile for other file types). To fix the issue, the exists check should probably be moved to Operations.rename.
EmmanuelMess
added
the
PR-Requested-Changes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem:
The rename completion path in
MainActivityHelpermay callnewFile.exists(context)on the UI thread after a rename attempt. For SMB targets that falls intoHybridFile.exists()and performs synchronousSmbFile.exists()network I/O on the main thread.Root cause:
The post-rename existence probe was added as a SAF workaround because
DocumentFile.renameTo()can report false even when the rename succeeds, but the fallback was applied uniformly to all backends.Fix:
Limit the post-rename
exists(context)verification to SAF-backed targets (DOCUMENT_FILEandOTG) and skip that UI-thread probe for SMB and other non-SAF modes.Closes #4610